aosp: wrap alarm() and make it use a thread-directed timer - #11340
aosp: wrap alarm() and make it use a thread-directed timer#11340rogerzanoni wants to merge 1 commit into
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a thread-directed alarm() wrapper (__abi_wrap_alarm) for Android in modular Starboard to ensure reliable delivery of SIGALRM signals using thread-local POSIX timers. Feedback on the implementation suggests optimizing the case where seconds is 0 by bypassing timer creation, simplifying the gettid() retrieval since it is always available on Android, and switching from CLOCK_MONOTONIC to CLOCK_REALTIME to align with standard POSIX alarm() behavior during device suspend.
| // A one-shot POSIX timer used to implement a thread-directed alarm() | ||
| struct ThreadAlarmTimer { | ||
| timer_t timer = nullptr; | ||
| bool created = false; |
There was a problem hiding this comment.
Is this necessary? Can we just check if timer is not nullptr instead?
|
@sacuff @jellefoks ptal |
| // A one-shot POSIX timer used to implement a thread-directed alarm() | ||
| struct ThreadAlarmTimer { | ||
| timer_t timer = nullptr; | ||
| bool created = false; |
Bionic's implementation of alarm triggers a setitimer set with ITIMER_REAL
making it a process-directed signal.
From the signal(7) manpage:
A process-directed signal may be delivered to any one of the threads
that does not currently have the signal blocked. If more than one of the
threads has the signal unblocked, then the kernel chooses an arbitrary
thread to which to deliver the signal.
Android's ART runtime/runtime.cc configures only a small set of signals:
void Runtime::BlockSignals() {
SignalSet signals;
signals.Add(SIGPIPE);
signals.Add(SIGQUIT);
signals.Add(SIGUSR1);
signals.Block();
}
SIGALRM is unblocked on the ART's main thread and it may catch the
process-directed signal, never letting it reach cobalt/nplb calling threads,
making it unreliable to wait for the signal using musl's implementation.
Fixes PosixClockNanosleepTest.ErrorEintrAbsoluteSleep,
PosixClockNanosleepTest.ErrorEintrRelativeSleep and
PosixNanosleepTests.ErrorEintr which were failing with a "signal never
interrupted sleep" error.
Bug: 532068409
| } | ||
|
|
||
| // matches musl's implementation return logic, but using | ||
| // nsec instead of usec |
There was a problem hiding this comment.
Why do we use nsec instead of usec?
There was a problem hiding this comment.
IIUC timer_settime takes itimerspec args and that struct uses nsec, vs musl's version uses setitimer with itimerval which has usec. setitimer was removed from posix so the change from musl to here is reasonable IMO.
I'd say the comment referencing the nsec/usec difference from musl is burying the lede a bit. It can probably just explain that we match the behavior to round up any partial seconds that are left on the timer.
3 participants
Bionic's implementation of alarm triggers a setitimer set with ITIMER_REAL making it a process-directed signal.
From the signal(7) manpage:
A process-directed signal may be delivered to any one of the threads
that does not currently have the signal blocked. If more than one of the
threads has the signal unblocked, then the kernel chooses an arbitrary
thread to which to deliver the signal.
Android's ART runtime/runtime.cc configures only a small set of signals:
void Runtime::BlockSignals() {
SignalSet signals;
signals.Add(SIGPIPE);
signals.Add(SIGQUIT);
signals.Add(SIGUSR1);
signals.Block();
}
SIGALRM is unblocked on the ART's main thread and it may catch the process-directed signal, never letting it reach cobalt/nplb calling threads, making it unreliable to wait for the signal using musl's implementation.
Fixes PosixClockNanosleepTest.ErrorEintrAbsoluteSleep, PosixClockNanosleepTest.ErrorEintrRelativeSleep and PosixNanosleepTests.ErrorEintr which were failing with a "signal never interrupted sleep" error.
Bug: 532068409